Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add banner configurable via module API #10321

Closed
wants to merge 5 commits into from
Closed

Add banner configurable via module API #10321

wants to merge 5 commits into from

Conversation

maheichyk
Copy link
Contributor

@maheichyk maheichyk commented Mar 8, 2023 •
edited by github-actions bot
Loading

Type: Enhancement
Related: https://github.com/matrix-org/matrix-react-sdk-module-api

This PR suggests to add possibility to configure a banner at the top of Element that is configurable via module API.

This could be used to show custom menu, widget toggle buttons or other information that makes sense in custom fork deployments.

There is no module API for that currently but could look like this:

export enum BannerLifecycle {
    Banner = "banner",
}

export type BannerOpts = {
    banner: JSX.Element | undefined;
};

export type BannerListener = (opts: BannerOpts) => void;

Module demo that shows simply button in the banner:

protected onBanner: BannerListener = (opts) => {
    opts.banner = <button>Custom Menu</button>;
};

And look the following:
Screenshot 2023-03-08 at 10 45 53

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

Add banner configurable via module API
9024187 
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements labels Mar 8, 2023
@maheichyk maheichyk marked this pull request as ready for review March 8, 2023 07:51
@maheichyk maheichyk requested a review from a team as a code owner March 8, 2023 07:51
@maheichyk
Copy link
Contributor Author

Would be great to hear some feedback on this proposal.

@t3chguy t3chguy added X-Blocked The PR cannot move forward in any capacity until an action is made and removed X-Blocked The PR cannot move forward in any capacity until an action is made labels Mar 8, 2023
@t3chguy t3chguy requested review from a team and removed request for germain-gg and t3chguy March 8, 2023 08:53
@maheichyk
Copy link
Contributor Author

Could you please provide some feedback on this proposal?

germain-gg
germain-gg suggested changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a technical point of view, I believe this can be a bit leaner as the HTML structure already exists and could be re-used.

We would need a test validating that a banner can be injected in the page too

But more importantly this requires a product review

src/components/structures/MatrixChat.tsx Outdated
ModuleRunner.instance.invoke(BannerLifecycle.Banner, opts);
if (opts.banner) {
view = (
<div className="mx_MatrixChat_wrapper">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an implementation point of view, I don't think we would need to add a wrapper as the banner could be slotted as a sibling of the MatrixChat and the parent section id="matrixchat" is a flex container that applies the same CSS that you declared for mx_MatrixChat_wrapper.
I'd be advocating for a leaner HTML structure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed implementation to place the Banner as a sibling of the mx_MatrixChat. Had to update existing mx_MatrixChat_wrapper with flex-direction: column for the Banner to be shown on the top of Element.

I have created a PR to the module API with a suggestion to add the Banner: matrix-org/matrix-react-sdk-module-api#13

@daniellekirkwood daniellekirkwood self-assigned this Mar 13, 2023
@daniellekirkwood
Copy link

👋 Product here...

Regular users would not see this? This is just available for power users & people running a fork..?

@maheichyk
Copy link
Contributor Author

👋 Product here...

Regular users would not see this? This is just available for power users & people running a fork..?

Hello 👋 yes, nothing is changed for the regular users. This is intended to be used in custom Element deployments. Currently we have to fork Element and apply custom changes. With this change in place, we could write a module that will add the banner useful in our deployment.

@t3chguy
Copy link
Member

t3chguy commented Mar 13, 2023

The product consideration here is one of bandwidth & maintenance, by adding this to the module API we have to ensure we don't break it and maintain it if someone else does

@daniellekirkwood
Copy link

Yes, comfortable with this. Thanks both!

@benparsons benparsons added the A-Timesheet-1 Log any time spent on this into the A-Timesheet-1 project label Mar 20, 2023
Mikhail Aheichyk added 2 commits March 21, 2023 20:11
@maheichyk maheichyk mentioned this pull request Mar 22, 2023
Merged
@daniellekirkwood daniellekirkwood removed their assignment Mar 27, 2023
@weeman1337 weeman1337 self-requested a review April 20, 2023 07:19
@weeman1337
Copy link
Contributor

@maheichyk can you update the code here and fix the tests? I would then have a look at your PRs.

Merge branch 'develop' into banner_module_api
d694b2f 
# Conflicts:
#	src/components/structures/LoggedInView.tsx
@maheichyk
Copy link
Contributor Author

Another PR is created to element-web: element-hq/element-web#25537. It is based on ideas from this comment: #10321. Element web seems to be a better place for these changes, please have a look.

@germain-gg
Copy link
Contributor

Superseeded by element-hq/element-web#25537

@germain-gg germain-gg closed this Jul 19, 2023
@dhenneke dhenneke deleted the banner_module_api branch August 22, 2023 07:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@germain-gg germain-gg germain-gg requested changes

@daniellekirkwood daniellekirkwood daniellekirkwood approved these changes

@weeman1337 weeman1337 Awaiting requested review from weeman1337

Assignees
No one assigned
Labels
A-Timesheet-1 Log any time spent on this into the A-Timesheet-1 project T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maheichyk @daniellekirkwood @t3chguy @weeman1337 @germain-gg @benparsons